[58105] Added update_item and delete_branch methods to HierarchicalItemService #16939
Conversation
app/services/custom_fields/hierarchy/hierarchical_item_service.rb
Outdated
Show resolved
Hide resolved
| item = CustomField::Hierarchy::Item | ||
| .create(parent: validation[:parent], label: validation[:label], short: validation[:short]) | ||
| def create_child_item(validation:) | ||
| item = validation[:parent].children.create(label: validation[:label], short: validation[:short]) |
There was a problem hiding this comment.
🟡 Hmm, not sure, but I do not like this change. What is the benefit? Before the code was very clear, that a new Item is created. Now I must understand, that the item to create is referenced here as .children.
🔴 Does this even work, if the created item is not the only child?
There was a problem hiding this comment.
Because that's how closure_tree works. It keeps intermediate representations of the tree for lookup, so we need to go through its code.
From another perspective, it is clearer to the random RoR dev as children.create maps to "I'm creating some associated record to the caller". :P
There was a problem hiding this comment.
Can you confirm, that this does not break, if the parent already has persisted children, when adding a new one?
There was a problem hiding this comment.
Yes. It works. Why? Because .children is an association proxy that don't override anything but ensure that the fields for the association are correctly set - and in case of closure tree, sets up the rest of the fields and optimizations.
spec/services/custom_fields/hierarchy/hierarchical_item_service_spec.rb
Outdated
Show resolved
Hide resolved
# Conflicts: # app/contracts/custom_fields/hierarchy/insert_item_contract.rb # spec/contracts/custom_fields/hierarchy/insert_item_contract_spec.rb
mereghost
force-pushed
the
implementation/58105-item-persistence-service
branch
from
103b927 to
160b3e6
Compare
app/contracts/custom_fields/hierarchy/generate_root_contract.rb
Outdated
Show resolved
Hide resolved
mereghost
force-pushed
the
implementation/58105-item-persistence-service
branch
from
35083ad to
2e27526
Compare
mereghost
force-pushed
the
implementation/58105-item-persistence-service
branch
from
2e27526 to
9670a16
Compare
Ticket
https://community.openproject.org/projects/openproject/work_packages/58105/github
What are you trying to accomplish?
See headline
What approach did you choose and why?
same as last time
Merge checklist